feat: Avatar and AvatarNameLabel EDS 2.0 - #5089
Conversation
|
@claude please review |
|
Claude finished @millus's task in 2m 36s —— View job Code review: Avatar & AvatarNameLabel
Nicely structured PR — file layout, 🔴 Accessibility — the initial has no meaning to a screen readerThis is the main gap. A standalone Worse, inside Recommendation:
This also makes the 🟠 Horizontal / vertical spacing tokens look swapped —
|
millus
changed the title
|
@claude please review |
|
Claude finished @millus's task in 1m 22s —— View job Code review: Avatar & AvatarNameLabel (re-review)
Good progress since the last pass — commit ✅ Resolved since last review
🟡 Remaining (all minor — non-blocking)
Overall this is in good shape — the previously-blocking a11y issue is fixed and the token/axis corrections are right. The remaining items are all polish and can land in this PR or a follow-up. Nice work. ✅ |
Initial implementation of Avatar (initial-based, three sizes, two emphasis levels, optional notification dot) and AvatarNameLabel (horizontal and vertical layouts) based on Figma designs.
- Add `name` prop: auto-derives initials and sets role="img" + aria-label
- Merge notification into avatar aria-label ("Ada Lovelace, notification")
to fix nested role="img" violation
- Make Avatar inside AvatarNameLabel decorative — fullName text provides context
- Rename `email` → `meta` for generic secondary label support
- Switch AvatarNameLabel to display:flex; truncate long names, wrap meta
- Derive two-letter initials from first+last word of fullName
- Remove data-testid from component internals; use role/text queries in tests
- Add slot story with overflow menu, role badge, and timestamp examples - Add edge cases story showing long name wrapping and multi-part initials - Add argTypes with categories to Avatar and AvatarNameLabel stories - Switch AvatarNameLabel to display:flex; long names wrap naturally - Remove overflow:hidden/ellipsis from full-name — no descender clipping - Remove unused --_notification-bg variable and duplicate font-family - Consolidate max-width into shared selector, use align-items:stretch - Restore text-box:trim-both cap alphabetic with correct scoping
- Add src and alt props — renders circular profile photo with object-fit:cover - When src is provided, img element carries the accessible name via alt - Initials are not shown when a photo is present - Notification dot still renders on photo avatars - Add Photo story and two tests (renders img, hides initials) - Update Figma Code Connect to handle Photo variant
…o NameLabel
- Rename AvatarNameLabel fullName prop to name for API consistency with Avatar
- Add src and alt props to AvatarNameLabel — passes through to inner Avatar
- Update Figma Code Connect to map figma.string('Full Name') to name prop
- Update all tests, stories, types, and descriptions to match
- Add NameLabelWithPhoto story and update slot story with real photos
- Add src/alt to argTypes under Core category
- Reorder stories: Notification before Photo on both Avatar and NameLabel
- Add size and emphasis pass-through tests for AvatarNameLabel - Add size, emphasis, notification to AvatarNameLabel Figma Code Connect
|
@claude please review |
|
Claude finished @millus's task in 1m 41s —— View job Code review: Avatar & AvatarNameLabel (re-review)
Since the last pass the PR added photo/image support and renamed 🟠 Photo inside
|
…CSS, Figma - Photo inside AvatarNameLabel is now decorative (alt="" by default) — avoids duplicate name announcement next to visible fullName text - deriveInitials guards against empty/whitespace name to prevent TypeError - Add axe test for AvatarNameLabel with photo - Fix notification JSDoc to accurately describe when/how it is announced - Add overflow-wrap:break-word to .full-name for consistency with .meta - Add Photo/src variant to AvatarNameLabel Figma Code Connect - Document why size/emphasis pass-through tests are smoke tests
|
@claude please re-review, is this good to go now? no blockers, ready for human review? |
|
Claude finished @millus's task in 1m 13s —— View job Re-review: Avatar & AvatarNameLabel — go/no-go
Verdict: ✅ No blockers — ready for human review. Every item flagged in the last review is resolved, and all 5 CI checks are green (Checks, Code Quality, PR #5089, React 18 compatibility, PR Title Check). ✅ Resolved since last pass (commit
|
…ls, token/JSDoc fixes - Add visually-hidden .notification-label span in AvatarNameLabel so screen readers announce notification status alongside the visible name - Remove export from deriveInitials in Avatar.tsx — internal detail, now duplicated locally in AvatarNameLabel to avoid leaky coupling - Revert gap token to --eds-container-gap-horizontal (semantically correct for a horizontal row arrangement) - Clarify --_color comment in avatar.css - Fix stale alt JSDoc in AvatarNameLabel.types.ts - Add axe + presence tests for notification in AvatarNameLabel
millus
modified the milestone:
[2026-Q2] Support AI-assisted design and development in Equinor 🤖
pomfrida
left a comment
pomfrida
left a comment
There was a problem hiding this comment.
Really solid PR overall. The component split between Avatar and AvatarNameLabel is clean, the accessibility model is well-thought-out across both contexts (the notification merged into aria-label for standalone, visually-hidden span for the composed component — two different solutions for two different contexts, and both right), the pseudo-private --_ variable pattern is applied correctly for size scaling, and the Figma Code Connect coverage for both components (including the Photo variant) is more than the bar. Tests are organised into Rendering / Variants / Notification / Accessibility blocks with a jest-axe assertion per meaningful variant — exactly what AGENTS.md asks for. Thanks also for pre-flagging the --eds-selectable-space-* deviation in the description.
A few inline comments split across three buckets:
- 🔴 Blockers (please address before merge):
deriveInitialsduplicated across both files (1), and thesize/emphasispass-through tests inAvatarNameLabeldon't actually assert anything (2) - 🟡 Worth a look: vertical-layout overflow at narrow viewports with long names (3, verified in Storybook with a concrete CSS fix proposed), generic
data-sizeattribute name (4), and a few smaller a11y / token observations - 🟢 Nits / follow-up: stylistic cleanups and one broader observation about hardcoded values in
/nextstories that isn't specific to this PR
| function deriveInitials(name: string): string { | ||
| const words = name.trim().split(/\s+/) | ||
| if (!words[0]) return '' | ||
| if (words.length === 1) return words[0][0].toUpperCase() | ||
| return (words[0][0] + words[words.length - 1][0]).toUpperCase() | ||
| } |
There was a problem hiding this comment.
deriveInitials is defined identically in both Avatar.tsx and AvatarNameLabel.tsx. Move it to a single source so the two can't drift. Either Avatar/utils.ts (component-local) or components/next/utils/ (shared) — the rest of /next keeps small helpers component-local, so a utils.ts next to Avatar.tsx and an import from both files would match existing conventions.
| // size and emphasis flow to the inner Avatar via props — verified visually | ||
| // and by TypeScript. Testing Library's no-node-access rule prevents querying | ||
| // the inner div's data attributes directly; the type system enforces the wiring. | ||
| it('renders with size prop without error', () => { | ||
| render(<AvatarNameLabel name="Ada" size="sm" />) | ||
| expect(screen.getByText('Ada')).toBeInTheDocument() | ||
| }) | ||
|
|
||
| it('renders with emphasis prop without error', () => { | ||
| render(<AvatarNameLabel name="Ada" emphasis="high" />) | ||
| expect(screen.getByText('Ada')).toBeInTheDocument() | ||
| }) | ||
| }) | ||
|
|
There was a problem hiding this comment.
These tests only assert that the visible name still renders — they don't verify that size or emphasis actually reach the inner Avatar. The comment says "Testing Library's no-node-access rule prevents querying the inner div", but screen.getByText('AL').closest('.eds-avatar') (or a data-testid on the inner Avatar) would let us assert data-size / data-emphasis directly. As written, the inner Avatar could be rendered with hard-coded defaults and these tests would still pass.
Suggestion:
it('passes size to the inner Avatar', () => {
render(<AvatarNameLabel name="Ada Lovelace" size="sm" />)
const avatar = screen.getByText('AL').closest('.eds-avatar')
expect(avatar).toHaveAttribute('data-size', 'sm')
})| &[data-size='sm'] { | ||
| --_size: var(--eds-sizing-icon-xs); /* 16px */ | ||
| --_font-size: var(--eds-typography-ui-body-xs-font-size); | ||
| --_line-height: var(--eds-typography-ui-body-xs-line-height-default); | ||
| --_dot-size: 8px; |
There was a problem hiding this comment.
Hmm I see all other /next components scope their size attribute (data-icon-size, data-selectable-space). data-size is the first plain variant in /next and can collide with ancestor styles or future tokens that key off [data-size]. Suggest renaming to data-avatar-size for consistency. (data-emphasis is fine — Badge already uses it.)
| & .names { | ||
| flex: initial; | ||
| flex-direction: row; | ||
| flex-shrink: 0; |
There was a problem hiding this comment.
In data-layout="vertical" the .content and .names blocks both have flex-shrink: 0, with white-space: nowrap on .names and no truncation fallback. Verified in Storybook via the NameLabelEdgeCases story (which uses deliberately long names like "Bartholomew Featherstonehaugh"):
- At a 375px viewport, the
.namesrow is 44px wider than the body — the email gets clipped on the right edge (macOS hides the scrollbar by default, so visually it looks like the text just disappears; Windows / Linux / iOS would show a horizontal scrollbar on<body>) - At a 240px viewport, the row is 347px wide inside a 176px container — ~50% of the email content is hidden
Acknowledged that this only triggers with the combination of (a) a narrow container and (b) long names, and that the layout is documented as "intended for wider contexts". But once both conditions hit, the email is invisible with no indicator that more text exists, which is worse than truncating with an ellipsis (where the … at least signals "there's more"). Suggest adding a defensive truncation so the component degrades gracefully — note that both .content and .names need to be allowed to shrink, otherwise the truncation doesn't engage:
&[data-layout='vertical'] {
& .content {
flex: 1 1 auto; /* was: flex: initial */
flex-shrink: 1; /* was: 0 — must let parent shrink */
min-width: 0;
}
& .names {
flex: 1 1 auto; /* was: flex: initial */
flex-shrink: 1; /* was: 0 */
min-width: 0; /* required for text-overflow inside flex */
/* flex-direction: row, white-space: nowrap kept */
}
& .meta {
overflow: hidden;
text-overflow: ellipsis;
min-width: 0;
}
}Verified locally — with the patch applied at 375px viewport, body overflow drops from 44px to 0px and the email shows as "b.featherstoneha…". Alternative: drop white-space: nowrap to allow wrapping (preserves information at the cost of multi-line).
| --_line-height: var(--eds-typography-ui-body-md-line-height-default); | ||
|
|
||
| /* Dot sizes have no token equivalent in the design — hardcoded per Figma spec */ | ||
| --_dot-size: 12px; |
There was a problem hiding this comment.
Acknowledged in the comments as "no token equivalent". Could you eave hard-coded but maybe reference the Figma spec node id in the comment so future readers can verify, in case it would be added later
| size={size} | ||
| emphasis={emphasis} | ||
| src={src} | ||
| alt={alt ?? ''} |
There was a problem hiding this comment.
alt={alt ?? ''} is passed to the inner Avatar, but Avatar itself does alt={alt ?? name ?? ''} (line 36 of Avatar.tsx) — so the alt prop on AvatarNameLabel does work via override, but the doc comment in AvatarNameLabel.types.ts:27 says it defaults to "" so the visible name isn't announced twice. Good, but worth a one-line code comment here explaining why we force '' rather than letting Avatar's own fallback to name run, since the visible name covers the image.
| 'The `children` prop renders into a trailing slot to the right of the name. It is open — use it for anything contextual to that person, like an action button, a role badge, or a timestamp. The examples here are just starting points.', | ||
| }, | ||
| }, | ||
| } |
There was a problem hiding this comment.
Not a blocker for this PR — Avatar.stories.tsx is fully consistent with how the other /next stories (Button, Badge, Icon, Input) handle demo styling. Flagging it as a broader observation rather than a change request: across /next stories we routinely inline raw #hex colours and '12px' / '16px' pixel values for demo scaffolding (section headings, gaps, sample backgrounds). Examples outside this PR:
Icon.stories.tsx—color: '#666',background: '#f5f5f5'Input.stories.tsx—background: '#f7f7f7','--eds-color-neutral-1': '#fff'Badge.stories.tsx—fontSize: '14px',fontSize: '16px'
Storybook is one of the most-visible surfaces of the design system, so demo styling that bypasses the token system is a bit of a "do as I say, not as I do" signal. Worth a separate conversation/PR about whether we want to tighten this up across the board (an .sb-section-heading utility or a shared story-helper component would cover most of the repeated patterns). So I think I will add a task for conforming it to our tokens for us to tackle after we have landed the new token structure. No action needed in this PR.
| {...rest} | ||
| > | ||
| {src ? ( | ||
| <img className="photo" src={src} alt={alt ?? name ?? ''} /> |
There was a problem hiding this comment.
The CSS sizes the image with position: absolute; width: 100%; height: 100%, so layout is stable, but adding explicit width / height attributes (matching the rendered pixel size) helps browsers reserve space before CSS loads and is a small CLS improvement. Optional.
| expect(screen.getByText('X')).toBeInTheDocument() | ||
| }) | ||
|
|
||
| it('renders slot right when children provided', () => { |
There was a problem hiding this comment.
Test asserts that the slot content renders, but not that it ends up in .slot-right. A .toHaveClass check on the closest container would catch a regression where someone moved children into .names.
| <span className="full-name">{name}</span> | ||
| {meta && <span className="meta">{meta}</span>} | ||
| {notification && ( | ||
| <span className="notification-label">Notification</span> |
There was a problem hiding this comment.
Good a11y instinct: when the avatar is decorative inside AvatarNameLabel, the notification can't ride on the avatar's aria-label, so you've added a visually-hidden span instead. Two different solutions for two different contexts, and both are right.
2 participants
Resolves #4948
Summary
Adds two new EDS 2.0 components to the
/nextexport: Avatar and AvatarNameLabel.Avatar
A circular badge displaying a user's initials or profile photo.
Props
name— full name of the person. Auto-derives initials (first + last word → e.g."Ada Lovelace"→"AL") and setsrole="img"+aria-labelautomatically for screen readersinitial— explicit override for the displayed initial(s) (1–2 chars recommended)src— profile photo URL; renders a circular image withobject-fit: coverinstead of initialsalt— alt text for the photo; falls back tonamesize—sm(16px) |md(24px) |lg(32px)emphasis—low(muted accent bg) |high(emphasis bg, white text)notification— success-tone dot indicator at bottom-right; merged intoaria-labelwhennameis set (e.g."Ada Lovelace, notification")AvatarNameLabel
Composes Avatar with a name and optional metadata label. Accepts all Avatar props plus:
name— displayed as the primary label; also auto-derives the avatar initialmeta— secondary label (email, job title, or any short string)layout—horizontal(name + meta stacked, for lists) |vertical(name + meta inline, for headers/nav)src/alt— photo support, passed through to the inner Avatarchildren— open trailing slot for contextual content (overflow menu, role badge, timestamp, etc.)Accessibility
Avatar: passnameto make it announced to screen readers — no extra attributes neededAvatarinsideAvatarNameLabel: decorative (the visiblenametext provides context)"Ada Lovelace, notification") for standalone — insideAvatarNameLabela visually-hidden span announces it alongside the nameoverflow:hiddentruncation (avoids descender clipping withtext-boxtokens)Figma Code Connect
Connects both
Avatar(node 9319-5410) andAvatarNameLabel(node 9319-5429) including the Photo variant.Deviations from Figma
--eds-selectable-space-*gap tokens inAvatarNameLabel— Figma pairs--eds-selectable-space-horizontalwithhorizontallayout and--eds-selectable-space-verticalwithverticallayout. Our code uses them by gap axis direction (consistent with how Input usespadding-block/padding-inline). Both tokens are the same value so there is no visual difference — needs a decision on intended semantics.Test plan
Avatar.test.tsx)AvatarNameLabelcontexts🤖 Generated with Claude Code